This confused me until I went and looked at the implementation and docs. I guess without having a "shape" that represents the discriminant, we'd have to rebuild how TypeScript decides what it is.

Yeah, I agree that this is ugly. For comparison, here's how Zod does it:

const myUnion = z.discriminatedUnion("status", [
  z.object({ status: z.literal("success"), data: z.string() }),
  z.object({ status: z.literal("failed"), error: z.instanceof(Error) }),
]);

I suppose we could do something like:

function discriminatedUnion<
  DiscriminantKey extends keyof any,
  T extends {[d: DiscriminantKey]: string | number | boolean}
>(discriminantKey: DiscriminantKey, parseVariant: Parser<T>): DiscriminatedUnionBuilder {
  // ...
}

That would mirror how TypeScript thinks about discriminated unions. But the problem is, it wouldn't handle our versioned widget options. Those aren't (in TS's view) valid discriminated unions, since the discriminant, version.major, is a nested field. My current parser design allows us to treat them kind of like discriminated unions anyway, by splitting the choice of variant and the parsing of that variant into separate steps that can run arbitrary parsing logic.

If only version were simply a number (like it should have been all along), we could just do the thing that makes intuitive sense here. The curse of pre-TS code strikes again! :/

...and now, after writing all that out, I am tempted to create a special parser abstraction just for versioned widget options. Then we could simplify the one for discriminated unions to be more like Zod's API.

Something like:

const parseMyWidget = parseVersionedWidgetOptions(parseVersion2)
  .migratedFrom(parseVersion1, upgradeFromVersion1)
  .migratedFrom(parseVersion0, upgradeFromVersion0)
  .parser

Would it be useful to include the discriminant info here? Otherwise we don't know which union entry has bad data. 🤔

At (root {type: "ok"}).value -- expected number, but got "footer"

Maybe that gets too verbose.

I agree that would be better. The parser context currently doesn't have a way to track which variant of a union you're currently trying to parse, so I can't think of a way to add this info to the message easily.

I'll try adding a forVariant method to ParseContext, parallel to forSubtree. Maybe that will be easier than I'm imagining.

It'd be nice of this could mention all eligible types.

Would you mind adding this note and an example to this doc? https://khanacademy.atlassian.net/wiki/spaces/ENG/pages/3524690040/Content+Issues+for+Backfill

https://khanacademy.atlassian.net/wiki/spaces/ENG/pages/3524690040/Content+Issues+for+Backfill